Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow users to choose a different email for notifications #28422

Merged
merged 8 commits into from
Sep 10, 2021

Conversation

blizzz
Copy link
Member

@blizzz blizzz commented Aug 13, 2021

Screenshot_20210828_000844

Screenshot_20210820_210826

Getting and setting primary mail via provisioning API

curl -u $userid -X PUT -d 'key=notify_email' -d 'value=myname%40mydomain.com' -H 'OCS-APIRequest: true' https://my.nxt.cld/ocs/v2.php/cloud/users/$userid

(mind the address must be added as additional email address and also be confirmed)

curl -u $userid -X GET -H 'OCS-APIRequest: true' https://nc.zara/master/ocs/v2.php/cloud/users/$userid

@blizzz blizzz added this to the Nextcloud 23 milestone Aug 13, 2021
@blizzz blizzz force-pushed the enh/27465/notification-email branch 2 times, most recently from 6ede3a0 to 2e49000 Compare August 18, 2021 11:21
@blizzz blizzz force-pushed the enh/27465/notification-email branch 3 times, most recently from 4ae2959 to 51aefa3 Compare August 25, 2021 10:50
Copy link
Member

@LukasReschke LukasReschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool stuff and nice work 🎉

apps/provisioning_api/appinfo/routes.php Outdated Show resolved Hide resolved
throw new InvalidArgumentException('Logged in user is not mail address owner');
}
$email = $this->crypto->decrypt($key);
$ref = \substr(hash('sha256', $email), 0, 8);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason we limit it to the first 8 chars here? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider it sufficient to avoid collisions – this is just part of the configkey stored in the db and used to avoid collisions only. If you manage to craft a collision – mind it applies per user – you manage to overwrite a previously stored token. It could have a security implication, if you manage to create a token for a different user that fits the collision, and yet the payload still needs to be valid and pass the checks.

lib/public/Accounts/IAccountProperty.php Show resolved Hide resolved
@blizzz blizzz force-pushed the enh/27465/notification-email branch 3 times, most recently from 38a7645 to c05a302 Compare August 30, 2021 22:33
@skjnldsv
Copy link
Member

skjnldsv commented Sep 1, 2021

/backport to stable22

@blizzz blizzz force-pushed the enh/27465/notification-email branch from c05a302 to 1b7519f Compare September 1, 2021 13:40
@blizzz blizzz marked this pull request as ready for review September 1, 2021 13:41
@blizzz blizzz force-pushed the enh/27465/notification-email branch from 1b7519f to 0c4dcdd Compare September 1, 2021 13:49
@blizzz blizzz added the pending documentation This pull request needs an associated documentation update label Sep 1, 2021
- to make it reusable
- needed for local email verification

Signed-off-by: Arthur Schiwon <[email protected]>
- mails added by (sub)admins are automatically verified
- provisioning_api controller as verification endpoint
- IAccountProperty gets a locallyVerified property
- IPropertyCollection gets a method to fetch an IAccountProperty by value
  - an remove equivalent was already present
- AccountManager always initiates mail verification on update if necessary
- add core success template for arbitrary title and message

Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz blizzz force-pushed the enh/27465/notification-email branch from beb22ea to 8c3553f Compare September 9, 2021 13:20
@blizzz blizzz requested a review from artonge September 9, 2021 13:21
@blizzz blizzz requested review from LukasReschke, a team, PVince81, ArtificialOwl and skjnldsv and removed request for a team September 9, 2021 13:21
@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 9, 2021
@blizzz blizzz force-pushed the enh/27465/notification-email branch from 8c3553f to d378fc7 Compare September 9, 2021 14:52
@blizzz blizzz force-pushed the enh/27465/notification-email branch from d378fc7 to 763136a Compare September 9, 2021 17:22
@blizzz blizzz requested a review from Pytal September 9, 2021 17:23
- this is to avoid automatic confirmation by certain softwares that open
  links

Signed-off-by: Arthur Schiwon <[email protected]>
- specific getters and setters on IUser and implementation
- new notify_email field in provisioning API

Signed-off-by: Arthur Schiwon <[email protected]>
- there will be times when it is necessary to reset this value for sure

Signed-off-by: Arthur Schiwon <[email protected]>
Copy link
Member

@Pytal Pytal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comprehensive 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: users and groups pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants